Skip to content

TIDO-508 Add extracted test SAML Attributes to proxy #4

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dmartinez
Copy link
Collaborator

No description provided.

@dmartinez dmartinez added the enhancement New feature or request label Aug 28, 2024
@dmartinez dmartinez requested a review from dshafer August 28, 2024 17:38
@dmartinez dmartinez self-assigned this Aug 28, 2024
@dmartinez dmartinez marked this pull request as draft August 30, 2024 21:38
@dmartinez dmartinez marked this pull request as ready for review September 3, 2024 20:34
Copy link
Contributor

@dshafer dshafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good-- just a couple comments that came to mind. LMKWYT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we rename the headers from "X-Forwarded-*" to "Shib-Proxy-{attributeName}"? For instance:

RequestHeader unset Shib-Proxy-displayName
RequestHeader unset Shib-Proxy-eppn
RequestHeader unset Shib-Proxy-mail

A couple reasons:

  1. RFC 6648 deprecated the "X-" prefix for custom HTTP headers. Instead we're just supposed to pick a sensible prefix (e.g. "Shib-Proxy-").
  2. As we pass more attributes through the proxy, it could get confusing if the attribute names in the headers don't match the SAML attribute names (Email vs. mail, DisplayName vs. displayName, User vs. eppn, etc.).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file contained customizations at one point, but they weren't needed anymore after you updated the attribute name format. Now the only changes look like they're changes in whitespace. Could we remove this file, and just use the version from the base image?

Sign in to join this conversation on GitHub.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants